Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🦋Update provider account Invitations index page #3923

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

josemigallas
Copy link
Contributor

@josemigallas josemigallas self-assigned this Oct 15, 2024
flash[:success] = 'Invitation will be resent soon.'
redirect_to provider_admin_account_invitations_path
end
format.xml { head :ok }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is not used anymore?

Copy link
Contributor Author

@josemigallas josemigallas Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not really but it was added 10+ years ago and I think XML responses are only used by API endpoints.

There is nothing about it in 3scale API docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UPDATE: After some digging I can't really justify removing this response so I will restore it and add a comment for posterity.

@josemigallas josemigallas force-pushed the THREESCALE-9883_invitations_index branch from 3e8332c to 3d3d5af Compare October 18, 2024 13:12
Comment on lines +28 to +39
# def sent_date(invitation)
# invitation.sent_at&.to_s(:long) || 'Not sent yet'
# end

# def status(invitation)
# invitation_status(invitation)
# # if invitation.accepted?
# # "yes, on #{invitation.accepted_at.to_s(:short)}"
# # else
# # 'no'
# # end
# end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better don't push commented code IMO

# end

def can_manage_invitation?(invitation)
@can_manage_invitation ||= @ability.can?(:manage, invitation)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong, after the first call, it will cache the value of @ability.can?(:manage, invitation) and return it on subsequent calls even when called with different invitation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would actually work in this case, because for the provider users the abilities are for the whole class, there is no condition that would make it different for different invitations - it's either allowed for all, or not allowed for all.

But I agree that still it's not right 😬

@@ -1,3 +1,6 @@
# frozen_string_literal: true

# TODO: remove and use presenter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are only two small methods left here, why not move them to the presenter directly instead of leaving a TODO?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants